Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(chain): Introduce tx_graph::Update WIP #1553

Closed
wants to merge 1 commit into from

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Aug 13, 2024

Description

Introduces the tx_graph::Update struct, which will be used to update a TxGraph. This will replace the old method of updating a TxGraph with a TxGraph.

Notes to the reviewers

With the current implementation, there is some goofiness in the esplora/electrum wallet examples and test_electrum::sync_with_electrum(), where an Update is converted into a TxGraph to execute update_last_seen_unconfirmed(), before reverting the TxGraph back into an Update again. Implementing #1550 would prevent the need for this.

Changelog notice

WIP

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@LagginTimes LagginTimes marked this pull request as draft August 13, 2024 15:47
@evanlinjin
Copy link
Member

@LagginTimes forgot to mention that this is step 1 for #1543

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
crates/chain/src/tx_graph.rs Show resolved Hide resolved
@notmandatory notmandatory added this to the 1.0.0-beta milestone Aug 13, 2024
@notmandatory
Copy link
Member

notmandatory commented Aug 13, 2024

I would like to avoid unnecessary refactoring like this for the 1.0 beta milestone, see my comment in #1543.

@evanlinjin
Copy link
Member

@notmandatory I suggested this change because I thought it was necessary for 1.0. Please refer to @LLFourn's comment here: #1543 (comment)

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
Comment on lines 753 to 764
pub struct Update<A> {
whole_txs: HashMap<Txid, Arc<Transaction>>,
partial_txs: HashMap<OutPoint, TxOut>,
last_seen: HashMap<Txid, u64>,
anchors: BTreeSet<(A, Txid)>,
}
Copy link
Member

@evanlinjin evanlinjin Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember that we will be moving Update into a separate crate bdk_core so we won't have access to private members.

Additionally, we don't want to provide floating outputs to our update logic when we already have the full tx in the same update struct.

With these two requirements in mind, let's see various solutions.

  • Publicize all members. This is a bad idea, since we can't enforce our invariances.
  • Enforce invariance when caller inserts data. When inserting full tx, rm all associated floating txouts of the tx's txid. Don't insert floating txout when there is a full tx of same txid.
  • Enforce invariance when extracting data. When extracting full txs, also rm floating txouts with the same txid. When extracting floating txouts, ignore elements which have an associated full tx.

I think enforcing invariance when caller inserts data is easier to reason with.

@LagginTimes LagginTimes force-pushed the graph_update branch 3 times, most recently from 170e8ba to 6818d79 Compare August 19, 2024 10:19
@notmandatory
Copy link
Member

This is replaced by #1568 right?

@evanlinjin
Copy link
Member

Replaced by #1568

@evanlinjin evanlinjin closed this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants